Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merge query parameters coming from path with params argument #647

Merged
merged 1 commit into from
May 7, 2018
Merged

Merge query parameters coming from path with params argument #647

merged 1 commit into from
May 7, 2018

Conversation

brandur
Copy link
Contributor

@brandur brandur commented May 7, 2018 •
edited by brandur-stripe
Loading

If specifying both query parameters in a path/URL down to Faraday (e.g.,
/v1/invoices/upcoming?coupon=25OFF) and query parameters in a hash
(e.g., { customer: "cus_123" }), it will silently overwrite the ones
in the path with the ones in the hash. This can cause problems where
some critical parameters are discarded and causes an error, as seen in
issue #646.

This patch modifies #execute_request so that before going out to
Faraday we check whether the incoming path has query parameters. If it
does, we decode them and add them to our query_params hash so that
all parameters from either place are preserved.

Fixes #646.

r? @remi-stripe
cc @stripe/api-libraries

@brandur-stripe brandur-stripe force-pushed the brandur-merge-query-params branch 2 times, most recently from 01ec2cd to fe21618 Compare May 7, 2018 20:25
@remi-stripe
Copy link
Contributor

The approach looks fine though I wonder if we should add a custom test for the List logic to ensure this is never lost too. It's not the first time we have a weird issue with the upcoming invoice and the fact that it has custom logic on the url and query params.

I'd also want to make sure we're not breaking some backwards compatibility logic with starting_after and ending_before which would be in the URL too if I'm not mistaken and could end up duplicated in the query params?

Also some tests are failing!

@brandur-stripe brandur-stripe force-pushed the brandur-merge-query-params branch from fe21618 to 6024fe3 Compare May 7, 2018 21:33
@stripe-ci
Copy link

Gerald Rule: Rubocop Core File Change

cc @pt-stripe

@brandur-stripe brandur-stripe force-pushed the brandur-merge-query-params branch from 6024fe3 to 0b41357 Compare May 7, 2018 21:48
@brandur
Merge query parameters coming from path with params argument
3a2724b 
If specifying both query parameters in a path/URL down to Faraday (e.g.,
`/v1/invoices/upcoming?coupon=25OFF`) _and_ query parameters in a hash
(e.g., `{ customer: "cus_123" }`), it will silently overwrite the ones
in the path with the ones in the hash. This can cause problems where
some critical parameters are discarded and causes an error, as seen in
issue #646.

This patch modifies `#execute_request` so that before going out to
Faraday we check whether the incoming path has query parameters. If it
does, we decode them and add them to our `query_params` hash so that
all parameters from either place are preserved.

Fixes #646.
@brandur-stripe brandur-stripe force-pushed the brandur-merge-query-params branch from 0b41357 to 3a2724b Compare May 7, 2018 21:51
@brandur-stripe
Copy link
Contributor

The approach looks fine though I wonder if we should add a custom test for the List logic to ensure this is never lost too. It's not the first time we have a weird issue with the upcoming invoice and the fact that it has custom logic on the url and query params.

Yeah, I see what you mean, but I kind of feel like we should just be testing this stuff where its implementation logic lives. It might have also been plausible to have put this in ListObject's implementation, but since it's in StripeClient, I kind of think that it makes sense just to put the tests there.

If .upcoming was doing anything custom, we'd probably want to test that as well, but it's really just relying on pretty standard list logic — the key here is that when a query parameter like customer= is sent to the API, the API reflects that parameter back to you in the response list object's url field. Your client is supposed to just agnostically use whatever value is in there as its next path target. That value containing query parameters is the unusual case, but it should still be handled in a general way.

I'd also want to make sure we're not breaking some backwards compatibility logic with starting_after and ending_before which would be in the URL too if I'm not mistaken and could end up duplicated in the query params?

I'm fairly confident this works, but added an extra test just in case that shows that when a parameter in params overrides one in the URL, the former is preferred. Want to take a look?

Also some tests are failing!

Rubocop :/ Fixed.

@remi-stripe
Copy link
Contributor

LGTM

@brandur-stripe
Copy link
Contributor

Thanks!

@brandur-stripe brandur-stripe merged commit 357ec52 into master May 7, 2018
@brandur-stripe brandur-stripe deleted the brandur-merge-query-params branch May 7, 2018 22:11
@brandur-stripe
Copy link
Contributor

Released as 3.13.1.

@myfitment
Copy link

I was just working to figure out where this broke, which was v3.8.2. Super impressed at how quickly you guys fixed this!

@brandur-stripe
Copy link
Contributor

@myfitment Thanks, and glad to help! I ran a test script to look at your problem specifically and this seemed to have fixed it pretty definitively, but let us know if you see any other trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@remi-stripe remi-stripe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brandur @remi-stripe @stripe-ci @brandur-stripe @myfitment